Merge lp:~bilalakhtar/unity/5.0series-sru-software-center-integration-fixes into lp:unity/5.0

[Bilal Akhtar]

SoftwareCenterLauncherIcon: avoid the "Waiting to Install" tooltip to stay after installation

Be consistent with auto-hiding launcher.

This branch fixes three bugs (linked to the branch), all regarding Software Center integration with the Unity launcher. These are bound for the next SRU, since all three of them are needed to fix bugs targeted at SRUs. One of the fixes, the one for bug #1012896, is the fix for an SRU regression.

The fix for bug #1012896 is needed only in the Precise branch. Here are the Unity quantal branch merge requests for the other two bug fixes:

https://code.launchpad.net/~bilalakhtar/unity/waiting-to-install-tooltip-925014/+merge/108858
https://code.launchpad.net/~bilalakhtar/unity/unhide-launcher-sc-fix-1002440/+merge/109501

[Tim Penhey (thumper)]

> if (path.empty() || !path.compare("software-center-agent"))

This needs to be changed, as in the other review.

review: Needs Fixing

[Bilal Akhtar (bilalakhtar)]

I've made the change, thanks!

[Gary Lasker (gary-lasker)]

Hi Bilal! Please be sure to test with the corresponding Software Center branch at lp:~gary-lasker/software-center/unity-launcher-integration-fixes to make sure that everything is working as you expect with this branch. Please let me know if anything happens that you are not expecting, or if you need to make any further changes in my branch to support the changes you've made here.

Thanks!!
Gary

[Bilal Akhtar (bilalakhtar)]

I tested my branch using Gary's branch of the Software Center changes, and it works as expected. The only thing that didn't work in the initial run was the urgent quirk, which I fixed in the latest commit. Now, even the urgent quirk gets set appropriately. So all is green from my end.

Can someone please review this branch? I'm willing to prepare a 5.12-0ubuntu1.2 upload with only this branch cherry-picked, if that's what Didier would like.

[Didier Roche-Tolomelli (didrocks)]

Hey Bilal, it's a little bit the same than the branch merged for Q, but this needs test to ensure there are no any additional regression to it in future updates

[Gary Lasker (gary-lasker)]

Hi Didier and Bilal. What I'm thinking is that, for this branch, only a very minimal set of tests should be required. The corresponding Software Center branch includes the bulk of the changes, and fixes quite a few high-target bugs (see https://code.launchpad.net/~gary-lasker/software-center/unity-launcher-integration-fixes), and for that reason I very much think that we ought to avoid any un-needed delay in getting this relatively straightforward SRU approved and released on the Unity side.

Note that for the Software Center side, the Unity integration tests are in place for all of this functionality.

Particularly, for these Unity-side changes, I think just adding unit tests for the specific changes in the branch would be enough. The reason that I say this is that the corresponding Software Center branch above actually takes back much of the functionality that was moved to the Unity side during Precise, and reverts to a design much closer to the launcher integration functionality that we had in previous releases.

Specifically, the Software Center branch above modifies the behavior such that the dbus call to the Unity launcher now occurs at the very *end* of the install (this is as it was in Natty and Oneiric). By doing this, we effectively bypass the requirement that Unity track the install progress display in the launcher icon, and we also avoid the need for Unity itself to determine the installed desktop file and the location of the icon.

So, considering that this fix from Bilal is very targeted and simple, and since much of the functionality on the Unity side has now been moved back to the Software Center side, I think a very small set of tests are all that is needed now. If and when we move back to having Unity monitor the apdaemon install events (see bug 1011681), then at that time the automatic test suite can and should be filled out with the corresponding unit tests for this functionality. But until this occurs, there isn't a need for Unity to test anything beyond the functionality represented by this branch.

Please let me know what I can do to help expedite this branch and get it out into an SRU. A very nice set of important bug fixes are depending on it.

Thanks Bilal, thanks didrocks!!

[Didier Roche-Tolomelli (didrocks)]

I agree with you, please ensure that this small set of tests is included into this branch (and the merge proposal to trunk as well). Thanks Gary :)

[Gary Lasker (gary-lasker)]

Hi again Didier and Bilal. So, I noticed in the Unity branch that there is a manual test for the Software Center/Unity launcher integration. However, this test would become incorrect with the merge of this branch, so I took the opportunity to update it and to fill it out with additional tests that will verify all of the fixes that are made by this branch along with the corresponding software-center branch at https://code.launchpad.net/~gary-lasker/software-center/unity-launcher-integration-fixes.

Here's the branch where I updated the manual tests in the file manual-tests/SoftwareCenter.txt.

  lp:~gary-lasker/unity/software-center-launcher-integration-tests

Now, I fully realize and appreciate that that manual tests are a "last resort" and even worse, according to the README, one should feel "shame" when adding them :), so it may well be that these are not appropriate for our use. However, it seems to me that these would serve as excellent functional tests that span the full integration interactions between Ubuntu Software Center and Unity to a degree that might not be very easy to capture in unit tests. For this reason, these manual tests seem to me to be quite useful.

Feel free to use them if you like, or just ignore them if they are not something that we want. If you'd like to use them, please feel free to just merge my branch into this one (as well as the trunk one) and make it all part of the same MP.

Many thanks!

[Bilal Akhtar (bilalakhtar)]

The manual tests in Gary's branch look good from my end. Can someone now review this branch again now that the tests are covered (in Gary's branch) ? Thanks!

[Didier Roche-Tolomelli (didrocks)]

Sorry and thanks Gary about the manual, this is good, but clearly not enough as we saw that this functionnality reverted a lot. I clearly want automated tests as this behavior regressed 3 times already, so clearly showing that the manual test wasn't enough in past. Bilal, glad to see you there, can you work on testing please?

Just to be clear: I won't accept this branch to distro as long as there is no automated tests.

[Gary Lasker (gary-lasker)]

Hi Didier, it's not a surprise that the manual tests were not enough because they were very sparse before (a single test and it tested only a cancelled installation). In other words, it didn't cover anything of the functionality that had regressed. Now, with my updates, they specify multiple test cases that include every detail that should be verified.

So I'm not sure the tests as updated can still be considered "clearly not enough", unless of course for some reason these aren't run or used, in which case I'd agree that no manual test will be useful.

The main benefit of manual tests for this feature is that the feature spans two different applications. We have full automatic tests on the Software Center side, but that is not sufficient to also test the Unity side, and the same will be true with automatic tests on the Unity side. The best we can test is the API between then, but whether the two operate correctly can be very effectively tested by a person.

I'm not saying that automatic tests should not also be included (automatic unit tests are always veryimportant), but assuming these manual tests are actually used and run, the set that I've added will be a very significant enhancement to any unit tests and should be sufficient to detect any regression on either side for this shared feature.

Thanks!

[Gary Lasker (gary-lasker)]

Per earlier discussion above and Didier's comment on 06-26, we need to add a small set of unit tests that specifically tests the changes in this branch.

[Bilal Akhtar (bilalakhtar)]

I've added an autopilot Unity test which is very similar to the one I wrote in March (in fact, it's the same code, just copied and with minor modifications). Appears that one got removed by mistake in the meantime, so I'm adding it again.

Thanks Didier for the feedback and Thomi/Chris for helping on figuring out the problems with the test, branch is ready for review again :)

[Thomi Richards (thomir-deactivatedaccount)]

The autopilot tests in this branch are fine. However, when merging this test into lp:unity trunk the test should make sue of a few additional autopilot features (Eventually matcher, lambda-based values) to remove the need for sleep() calls. Talk to me if you need help with this.

review: Approve (quality)

[Gary Lasker (gary-lasker)]

Hi Didier and Bilal. Shall we also add the "manual" tests from lp:~gary-lasker/unity/software-center-launcher-integration-tests to this merge? These would be very useful later as we start running the manual tests as they check very specific functionality of the integration between Software Center and the Unity launcher, per my description above.

Or is a separate MP more desireable for this (no-code-change) branch?

Many thanks,
Gary

[Didier Roche-Tolomelli (didrocks)]

@Gary: I'm happy with both. Please if you propose a separate MR, paste it here.

There is an unity SRU in preparation next week, would be good that upstream review both branches now…

[Gary Lasker (gary-lasker)]

@Didier: Ok, I've written a separate MR for the manual tests, and set it as depending on this branch as a prerequisite. Here's the MR:

  https://code.launchpad.net/~gary-lasker/unity/software-center-launcher-integration-tests-for-5.0/+merge/114241

How's it looking for this getting into an SRU this week? We'd like to coordinate the corresponding Software Center SRU as much as possible.

Thanks for all your help!

[Didier Roche-Tolomelli (didrocks)]

@Gary: I reasked the PS team to review the branches to get that in in this week SRU. However, your other branch is only proposed to lp:unity, not lp:unity/5.0. Didn't you want to propose it for the SRU branch as well?

[Gary Lasker (gary-lasker)]

@Didier: I believe my MR is targeted to lp:/unity/5.0, at least it says so here??

  https://code.launchpad.net/~gary-lasker/unity/software-center-launcher-integration-tests-for-5.0/+merge/114241

  "Merge into: lp:unity/5.0"

Please let me know if something isn't set correctly. Thanks! :)

[Didier Roche-Tolomelli (didrocks)]

@Gary: it's fine indeed, shold have taken more coffee ;)

[Gary Lasker (gary-lasker)]

@Didier: Haha! Ok, great, I just wanted to double-check that it wasn't *me* who needed the coffee! :) Thanks again Didier!

[Brandon Schaefer (brandontschaefer)]

Hmm I still get waiting to install... while any program is installing (not waiting to). Though the animation stuff is fixed.

[Gary Lasker (gary-lasker)]

Hi Brandon, and thanks very much for testing this. Actually, you need to be using the corresponding updated version of Software Center as well for the integration to fully work. This can be found in my branch here:

  lp:~gary-lasker/software-center/unity-launcher-integration-fixes

I have a test build of this currently building in my PPA so that it's easier to get and use, but feel free to use the branch directly. To run it, simply get a copy of the branch above and run as follows from the top level of the branch:

  $ ./software-center

I'll add a note here when my PPA build is complete.

Thanks again!
Gary

[Marco Trevisan (Treviño) (3v1n0)]

I don't think we can fix the string for 5.0 as it would break the l18n... So I'd accept this as it is.

[Gary Lasker (gary-lasker)]

Just fyi, I prepared a test version of Ubuntu Software Center that includes this functionality and it is now ready in my PPA at:

  https://launchpad.net/~gary-lasker/+archive/ppa

The version of interest is the one for Precise, version 5.2.5~unitylaunchertest1.

[Gary Lasker (gary-lasker)]

@Brandon, I think that you should no longer be seeing the "Waiting to Install" message when using the test version of Software Center, correct? I'm referring to your comment in the manual test case MR:

  https://code.launchpad.net/~gary-lasker/unity/software-center-launcher-integration-tests-for-5.0/+merge/114241

Thanks again!

[Brandon Schaefer (brandontschaefer)]

@Gary,

Well now I just don't get an installing animation at all...

[Bilal Akhtar (bilalakhtar)]

On Mon, Jul 16, 2012 at 2:13 PM, Brandon Schaefer
<email address hidden> wrote:
>
> Well now I just don't get an installing animation at all...

The animation now happens AFTER install. It's an intended change.

> --
> https://code.launchpad.net/~bilalakhtar/unity/5.0series-sru-software-center-integration-fixes/+merge/110214
> You are the owner of lp:~bilalakhtar/unity/5.0series-sru-software-center-integration-fixes.

[Brandon Schaefer (brandontschaefer)]

Ooo alright, well that will fix that problem :). Looks good then.

review: Approve

[Brandon Schaefer (brandontschaefer)]

Wasn't the problem because of a different used *.desktop file? Could you send 2 *.desktop strings to unity, the one for while it is installing then the second one to update that bamf app after it finishes? (I could be completely wrong also :) )

[Gary Lasker (gary-lasker)]

Yep! :) That would be my preferred fix as well, but I'd like to know how that sounds to Bilal. We could simply define a second dbus call so that we send the first at the start of the animation to initiate the animation, and then send the second at the end of install to provide the "real" desktop info. Bilal, is this feasible? It does seem the most straightforward fix as you would not have to do any path conversions at all on the Unity side.

[Łukasz Zemczak (sil2100)]

So we're waiting for Bilal to comment on the new proposition here? Or are we fine with the fix as it is now?

[Bilal Akhtar (bilalakhtar)]

The fix in its current state is good for Precise. The two DBus call fix would need changes on the SC side too and would be an unnecessary change for an SRU. So I'll make that change for 6.x while this is fit to enter 5.14

[Marco Trevisan (Treviño) (3v1n0)]

Mh, I don't know... Removing the installation animation until the installation process is done, we completely change the behavior from what we had in P, and could be easily identified as a "regression" (even if actually it's a feature).

If to fix bug #991926 we really can't send another .desktop file once the installation is done pointing to the real one (that was my best fix as well), we can workaround it the unity side by replacing the SoftwareCenterLauncherIcon with a standard BamfLauncherIcon that points to the real desktop when the installation is done. In this case we can only "guess" where the real desktop file is, but I think that hardcoding the path to /usr/share/applications can be quite safe.

[Bilal Akhtar (bilalakhtar)]

I don't think it will be identified as a regression. The animation is
almost completely broken in Precise at the moment, and there's the fact
that free universe apps won't need two dbus calls in the first place. The
issue is only with paid and ARB apps right now.
On Jul 17, 2012 7:10 AM, "Marco Trevisan (Treviño)" <[e-mail]> wrote:

> Mh, I don't know... Removing the installation animation until the
> installation process is done, we completely change the behavior from what
> we had in P, and could be easily identified as a "regression" (even if
> actually it's a feature).
>
> If to fix bug #991926 we really can't send another .desktop file once the
> installation is done pointing to the real one (that was my best fix as
> well), we can workaround it the unity side by replacing the
> SoftwareCenterLauncherIcon with a standard BamfLauncherIcon that points to
> the real desktop when the installation is done. In this case we can only
> "guess" where the real desktop file is, but I think that hardcoding the
> path to /usr/share/applications can be quite safe.
> --
>
> https://code.launchpad.net/~bilalakhtar/unity/5.0series-sru-software-center-integration-fixes/+merge/110214
> You are the owner of
> lp:~bilalakhtar/unity/5.0series-sru-software-center-integration-fixes.
>

[Marco Trevisan (Treviño) (3v1n0)]

Approving this for now, I hope to get this feature back in Q and backported in P as well.

review: Approve

=== modified file 'plugins/unityshell/src/Launcher.cpp'
--- plugins/unityshell/src/Launcher.cpp	2012-06-21 16:10:54 +0000
+++ plugins/unityshell/src/Launcher.cpp	2012-06-28 02:08:18 +0000
@@ -3030,6 +3030,8 @@
       &self->_sc_icon_aptdaemon_task, NULL);
 
     g_dbus_method_invocation_return_value(invocation, nullptr);
+
+    self->EnsureAnimation();
   }
 }
 

=== modified file 'plugins/unityshell/src/LauncherController.cpp'
--- plugins/unityshell/src/LauncherController.cpp	2012-06-21 17:30:08 +0000
+++ plugins/unityshell/src/LauncherController.cpp	2012-06-28 02:08:18 +0000
@@ -457,6 +457,11 @@
       return;
   }
 
+ // Check if desktop file was supplied, or if it's set to SC's agent
+ // See https://bugs.launchpad.net/unity/+bug/1002440
+  if (path.empty() || path == "software-center-agent")
+    return;
+
   SoftwareCenterLauncherIcon::Ptr result = CreateSCLauncherIcon(path, aptdaemon_trans_id, icon_path);
 
   launcher_->ForceReveal(true);

=== modified file 'plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp'
--- plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp	2012-04-17 09:03:31 +0000
+++ plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp	2012-06-28 02:08:18 +0000
@@ -43,6 +43,7 @@
                    G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START)
 , finished_(true)
 , needs_urgent_(false)
+, aptdaemon_trans_id_(aptdaemon_trans_id)
 {
 
   aptdaemon_trans_.Connect("PropertyChanged", sigc::mem_fun(this, &SoftwareCenterLauncherIcon::OnPropertyChanged));
@@ -50,7 +51,12 @@
 
   SetIconType(TYPE_APPLICATION);
   icon_name = icon_path;
-  tooltip_text = _("Waiting to install");
+  if (!aptdaemon_trans_id_.empty()) // Application is being installed, or hasn't been installed yet
+    tooltip_text = _("Waiting to install");
+  else {
+    SetQuirk(QUIRK_URGENT, true);
+    needs_urgent_ = true;
+  }
 }
 
 void SoftwareCenterLauncherIcon::Animate(nux::ObjectPtr<Launcher> launcher,

=== modified file 'plugins/unityshell/src/SoftwareCenterLauncherIcon.h'
--- plugins/unityshell/src/SoftwareCenterLauncherIcon.h	2012-04-17 09:03:31 +0000
+++ plugins/unityshell/src/SoftwareCenterLauncherIcon.h	2012-06-28 02:08:18 +0000
@@ -21,6 +21,7 @@
 #ifndef SOFTWARE_CENTER_LAUNCHERICON_H
 #define SOFTWARE_CENTER_LAUNCHERICON_H
 
+#include <string>
 #include <UnityCore/GLibDBusProxy.h>
 #include "BamfLauncherIcon.h"
 
@@ -61,6 +62,8 @@
   nux::ObjectPtr<Launcher> launcher_;
   bool finished_;
   bool needs_urgent_;
+
+  std::string aptdaemon_trans_id_;
 };
 
 }

=== modified file 'tests/autopilot/autopilot/tests/test_launcher.py'
--- tests/autopilot/autopilot/tests/test_launcher.py	2012-06-21 17:30:08 +0000
+++ tests/autopilot/autopilot/tests/test_launcher.py	2012-06-28 02:08:18 +0000
@@ -471,6 +471,33 @@
         self.assertThat(calc_icon, NotEquals(None))
         self.assertThat(calc_icon.visible, Eventually(Equals(True)))
 
+    def test_software_center_add_icon(self):
+        """ Test the ability to add a SoftwareCenterLauncherIcon """
+        sleep(.5)
+        
+        # Check if SC is pinned to the launcher already
+        icon = self.launcher.model.get_icon_by_desktop_id("ubuntu-software-center.desktop")
+        if icon != None:
+            self.launcher_instance.unlock_from_launcher(icon)
+            sleep(2.0)
+
+        self.launcher.add_launcher_item_from_position("Unity Test",
+                                                   "softwarecenter",
+                                                   100,
+                                                   100,
+                                                   32,
+                                                   "/usr/share/applications/ubuntu-software-center.desktop",
+                                                   "")
+        
+        sleep(2.0)
+
+        icon = self.launcher.model.get_icon_by_desktop_id("ubuntu-software-center.desktop")
+
+        # Check if the icon tooltip is NOT empty and NOT "waiting to install"
+        self.assertThat(icon.tooltip_text == "Waiting to install", Equals(False))
+        self.assertThat(len(icon.tooltip_text) == 0, Equals(False))
+        sleep(.5)
+
 class LauncherDragIconsBehavior(LauncherTestCase):
     """Tests interation with dragging icons with the Launcher"""
 
@@ -525,7 +552,6 @@
                      get_launcher_icons_for_monitor(self.launcher_monitor)[-3]
         self.assertThat(moved_icon.id, Equals(calc_icon.id))
 
-
 class LauncherRevealTests(LauncherTestCase):
     """Test the launcher reveal behavior when in autohide mode."""