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

[Gary Lasker]

* 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)

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)]

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

[lp:~gary-lasker/software-center/handle-trans-cancel-lp1027209]

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)]

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

[lp:~gary-lasker/software-center/handle-trans-cancel-lp1027209]

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)]

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!

[lp:~gary-lasker/software-center/handle-trans-cancel-lp1027209]

3185. By Gary Lasker on 2012-09-18

merge latest trunk

[Michael Vogt (mvo)]

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)]

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)]

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!

=== modified file 'softwarecenter/backend/installbackend_impl/aptd.py'
--- softwarecenter/backend/installbackend_impl/aptd.py	2012-09-05 06:46:15 +0000
+++ softwarecenter/backend/installbackend_impl/aptd.py	2012-09-18 04:57:18 +0000
@@ -193,6 +193,10 @@
                     'transaction-stopped': (GObject.SIGNAL_RUN_FIRST,
                                             GObject.TYPE_NONE,
                                             (GObject.TYPE_PYOBJECT,)),
+                    # emits a TransactionFinished object
+                    'transaction-cancelled': (GObject.SIGNAL_RUN_FIRST,
+                                              GObject.TYPE_NONE,
+                                              (GObject.TYPE_PYOBJECT,)),
                     # emits with a pending_transactions list object
                     'transactions-changed': (GObject.SIGNAL_RUN_FIRST,
                                              GObject.TYPE_NONE,
@@ -848,6 +852,14 @@
         self._logger.debug("_on_transaction_finished: %s %s %s" % (
                 trans, enum, trans.meta_data))
 
+        # first check if there has been a cancellation of
+        # the install and fire a transaction-cancelled signal
+        # (see LP: #1027209)
+        if enum == enums.EXIT_CANCELLED:
+            result = TransactionFinishedResult(trans, False)
+            self.emit("transaction-cancelled", result)
+            return
+
         # show error
         if enum == enums.EXIT_FAILED:
             # Handle invalid packages separately

=== modified file 'softwarecenter/ui/gtk3/panes/availablepane.py'
--- softwarecenter/ui/gtk3/panes/availablepane.py	2012-09-17 15:15:32 +0000
+++ softwarecenter/ui/gtk3/panes/availablepane.py	2012-09-18 04:57:18 +0000
@@ -227,8 +227,11 @@
             self.on_transactions_changed)
         self.backend.connect("transaction-finished",
             self.on_transaction_complete)
+        self.backend.connect("transaction-cancelled",
+            self.on_transaction_cancelled)
+        # a transaction error is treated the same as a cancellation
         self.backend.connect("transaction-stopped",
-            self.on_transaction_complete)
+            self.on_transaction_cancelled)
 
         # now we are initialized
         self.searchentry.set_sensitive(True)
@@ -392,11 +395,19 @@
             self._update_action_bar()
 
     def on_transaction_complete(self, backend, result):
+        """ handle a transaction that has completed successfully
+        """
         if result.pkgname in self.unity_launcher_transaction_queue:
             transaction_details = (
                 self.unity_launcher_transaction_queue.pop(result.pkgname))
             self._add_application_to_unity_launcher(transaction_details)
 
+    def on_transaction_cancelled(self, backend, result):
+        """ handle a transaction that has been cancelled
+        """
+        if result.pkgname in self.unity_launcher_transaction_queue:
+            self.unity_launcher_transaction_queue.pop(result.pkgname)
+
     def _register_unity_launcher_transaction_started(self, pkgname, appname,
                                                      trans_id, trans_type):
         # at the start of the transaction, we gather details for use later

=== 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-18 04:57:18 +0000
@@ -47,7 +47,8 @@
     def tearDownClass(cls):
         cls.win.destroy()
 
-    def _simulate_install_events(self, app):
+    def _simulate_install_events(self, app,
+                                 result_event="transaction-finished"):
         # pretend we started an install
         self.available_pane.backend.emit("transaction-started",
                                     app.pkgname, app.appname,
@@ -57,7 +58,7 @@
         # send the signal to complete the install
         mock_result = Mock()
         mock_result.pkgname = app.pkgname
-        self.available_pane.backend.emit("transaction-finished",
+        self.available_pane.backend.emit(result_event,
                                     mock_result)
         do_events_with_sleep()
 
@@ -122,6 +123,9 @@
         self.assertTrue(mock_send_application_to_launcher.called)
         args, kwargs = mock_send_application_to_launcher.call_args
         self._check_send_application_to_launcher_args(*args, **kwargs)
+        # insure that the unity launcher transaction queue is cleared
+        self.assertEqual(
+            self.available_pane.unity_launcher_transaction_queue, {})
 
     @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher'
            '.send_application_to_launcher')
@@ -141,6 +145,9 @@
         self.assertTrue(mock_send_application_to_launcher.called)
         args, kwargs = mock_send_application_to_launcher.call_args
         self._check_send_application_to_launcher_args(*args, **kwargs)
+        # insure that the unity launcher transaction queue is cleared
+        self.assertEqual(
+            self.available_pane.unity_launcher_transaction_queue, {})
         
     @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher'
            '.send_application_to_launcher')
@@ -151,6 +158,9 @@
         test_pkgname = "software-center"
         self._navigate_to_appdetails_and_install(test_pkgname)
         self.assertFalse(mock_send_application_to_launcher.called)
+        # insure that the unity launcher transaction queue is cleared
+        self.assertEqual(
+            self.available_pane.unity_launcher_transaction_queue, {})
        
     @patch('softwarecenter.ui.gtk3.panes.availablepane'
            '.convert_desktop_file_to_installed_location')
@@ -188,6 +198,64 @@
             # the previous call(s)
             mock_send_application_to_launcher.reset_mock()
 
+    @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher'
+           '.send_application_to_launcher')
+    def test_unity_launcher_integration_cancelled_install(self,
+                                         mock_send_application_to_launcher):
+        # test the automatic add to launcher enabled functionality when
+        # installing an app from the details view, and then cancelling
+        # the install (see LP: #1027209)
+        self.config.add_to_unity_launcher = True
+        test_pkgname = "software-center"
+        self.expected_pkgname = test_pkgname
+        self.expected_launcher_info = UnityLauncherInfo("software-center",
+                "softwarecenter",
+                0, 0, 0, 0, # these values are set in availablepane
+                "/usr/share/applications/ubuntu-software-center.desktop",
+                "")
+        app = Application("", test_pkgname)
+        self.available_pane.app_view.emit("application-activated",
+                                     app)
+        do_events()
+        self._simulate_install_events(app,
+                                      result_event="transaction-cancelled")
+        # ensure that we do not send the application to the launcher
+        # on a cancelled installation
+        self.assertFalse(mock_send_application_to_launcher.called)
+        # insure that the unity launcher transaction queue is cleared
+        self.assertEqual(
+            self.available_pane.unity_launcher_transaction_queue, {})
+        
+    @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher'
+           '.send_application_to_launcher')
+    def test_unity_launcher_integration_installation_failure(self,
+                                         mock_send_application_to_launcher):
+        # test the automatic add to launcher enabled functionality when
+        # a failure is detected during the transaction (aptd emits a
+        # "transaction-stopped" signal for this case)
+        self.config.add_to_unity_launcher = True
+        test_pkgname = "software-center"
+        self.expected_pkgname = test_pkgname
+        self.expected_launcher_info = UnityLauncherInfo("software-center",
+                "softwarecenter",
+                0, 0, 0, 0, # these values are set in availablepane
+                "/usr/share/applications/ubuntu-software-center.desktop",
+                "")
+        app = Application("", test_pkgname)
+        self.available_pane.app_view.emit("application-activated",
+                                     app)
+        do_events()
+        # aptd will emit a "transaction-stopped" signal if a transaction
+        # error is encountered
+        self._simulate_install_events(app,
+                                      result_event="transaction-stopped")
+        # ensure that we do not send the application to the launcher
+        # on a failed installation
+        self.assertFalse(mock_send_application_to_launcher.called)
+        # insure that the unity launcher transaction queue is cleared
+        self.assertEqual(
+            self.available_pane.unity_launcher_transaction_queue, {})
+
 
 class DesktopFilepathConversionTestCase(unittest.TestCase):