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

Proposed by Bilal Akhtar on 2012-06-14
Status: Merged
Approved by: Marco Trevisan (Treviño) on 2012-07-17
Approved revision: 2369
Merged at revision: 2389
Proposed branch: lp:~bilalakhtar/unity/5.0series-sru-software-center-integration-fixes
Merge into: lp:unity/5.0
Diff against target: 120 lines (+44/-2)
5 files modified
plugins/unityshell/src/Launcher.cpp (+2/-0)
plugins/unityshell/src/LauncherController.cpp (+5/-0)
plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp (+7/-1)
plugins/unityshell/src/SoftwareCenterLauncherIcon.h (+3/-0)
tests/autopilot/autopilot/tests/test_launcher.py (+27/-1)
To merge this branch: bzr merge lp:~bilalakhtar/unity/5.0series-sru-software-center-integration-fixes
Reviewer Review Type Date Requested Status
Marco Trevisan (Treviño) Approve on 2012-07-17
Brandon Schaefer (community) Approve on 2012-07-16
Thomi Richards (community) quality 2012-06-18 Approve on 2012-07-01
Tim Penhey (community) 2012-06-14 Needs Fixing on 2012-06-18
Review via email: mp+110214@code.launchpad.net

Commit message

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

Be consistent with auto-hiding launcher.

Description of the change

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

To post a comment you must log in.
Tim Penhey (thumper) wrote :

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

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

review: Needs Fixing
2366. By Bilal Akhtar on 2012-06-18

Fixed string comparison, thanks Tim

Bilal Akhtar (bilalakhtar) wrote :

I've made the change, thanks!

Gary Lasker (gary-lasker) wrote :

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

2367. By Bilal Akhtar on 2012-06-20

Fix urgent quirk after installation of paid app

Bilal Akhtar (bilalakhtar) wrote :

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 (didrocks) wrote :

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) wrote :

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 (didrocks) wrote :

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) wrote :

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) wrote :

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 (didrocks) wrote :

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) wrote :

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) wrote :

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.

2368. By Bilal Akhtar on 2012-06-28

Added test, need to probably fix it though

2369. By Bilal Akhtar on 2012-06-28

Merge from 5.0 branch

Bilal Akhtar (bilalakhtar) wrote :

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) wrote :

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) wrote :

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 (didrocks) wrote :

@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) wrote :

@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 (didrocks) wrote :

@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) wrote :

@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 (didrocks) wrote :

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

Gary Lasker (gary-lasker) wrote :

@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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

@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) wrote :

@Gary,

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

Bilal Akhtar (bilalakhtar) wrote :

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) wrote :

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

review: Approve
Brandon Schaefer (brandontschaefer) wrote :

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) wrote :

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) wrote :

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) wrote :

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

Bilal Akhtar (bilalakhtar) wrote :

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)" <mail@3v1n0.net> 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) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/unityshell/src/Launcher.cpp'
2--- plugins/unityshell/src/Launcher.cpp 2012-06-21 16:10:54 +0000
3+++ plugins/unityshell/src/Launcher.cpp 2012-06-28 02:08:18 +0000
4@@ -3030,6 +3030,8 @@
5 &self->_sc_icon_aptdaemon_task, NULL);
6
7 g_dbus_method_invocation_return_value(invocation, nullptr);
8+
9+ self->EnsureAnimation();
10 }
11 }
12
13
14=== modified file 'plugins/unityshell/src/LauncherController.cpp'
15--- plugins/unityshell/src/LauncherController.cpp 2012-06-21 17:30:08 +0000
16+++ plugins/unityshell/src/LauncherController.cpp 2012-06-28 02:08:18 +0000
17@@ -457,6 +457,11 @@
18 return;
19 }
20
21+ // Check if desktop file was supplied, or if it's set to SC's agent
22+ // See https://bugs.launchpad.net/unity/+bug/1002440
23+ if (path.empty() || path == "software-center-agent")
24+ return;
25+
26 SoftwareCenterLauncherIcon::Ptr result = CreateSCLauncherIcon(path, aptdaemon_trans_id, icon_path);
27
28 launcher_->ForceReveal(true);
29
30=== modified file 'plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp'
31--- plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp 2012-04-17 09:03:31 +0000
32+++ plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp 2012-06-28 02:08:18 +0000
33@@ -43,6 +43,7 @@
34 G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START)
35 , finished_(true)
36 , needs_urgent_(false)
37+, aptdaemon_trans_id_(aptdaemon_trans_id)
38 {
39
40 aptdaemon_trans_.Connect("PropertyChanged", sigc::mem_fun(this, &SoftwareCenterLauncherIcon::OnPropertyChanged));
41@@ -50,7 +51,12 @@
42
43 SetIconType(TYPE_APPLICATION);
44 icon_name = icon_path;
45- tooltip_text = _("Waiting to install");
46+ if (!aptdaemon_trans_id_.empty()) // Application is being installed, or hasn't been installed yet
47+ tooltip_text = _("Waiting to install");
48+ else {
49+ SetQuirk(QUIRK_URGENT, true);
50+ needs_urgent_ = true;
51+ }
52 }
53
54 void SoftwareCenterLauncherIcon::Animate(nux::ObjectPtr<Launcher> launcher,
55
56=== modified file 'plugins/unityshell/src/SoftwareCenterLauncherIcon.h'
57--- plugins/unityshell/src/SoftwareCenterLauncherIcon.h 2012-04-17 09:03:31 +0000
58+++ plugins/unityshell/src/SoftwareCenterLauncherIcon.h 2012-06-28 02:08:18 +0000
59@@ -21,6 +21,7 @@
60 #ifndef SOFTWARE_CENTER_LAUNCHERICON_H
61 #define SOFTWARE_CENTER_LAUNCHERICON_H
62
63+#include <string>
64 #include <UnityCore/GLibDBusProxy.h>
65 #include "BamfLauncherIcon.h"
66
67@@ -61,6 +62,8 @@
68 nux::ObjectPtr<Launcher> launcher_;
69 bool finished_;
70 bool needs_urgent_;
71+
72+ std::string aptdaemon_trans_id_;
73 };
74
75 }
76
77=== modified file 'tests/autopilot/autopilot/tests/test_launcher.py'
78--- tests/autopilot/autopilot/tests/test_launcher.py 2012-06-21 17:30:08 +0000
79+++ tests/autopilot/autopilot/tests/test_launcher.py 2012-06-28 02:08:18 +0000
80@@ -471,6 +471,33 @@
81 self.assertThat(calc_icon, NotEquals(None))
82 self.assertThat(calc_icon.visible, Eventually(Equals(True)))
83
84+ def test_software_center_add_icon(self):
85+ """ Test the ability to add a SoftwareCenterLauncherIcon """
86+ sleep(.5)
87+
88+ # Check if SC is pinned to the launcher already
89+ icon = self.launcher.model.get_icon_by_desktop_id("ubuntu-software-center.desktop")
90+ if icon != None:
91+ self.launcher_instance.unlock_from_launcher(icon)
92+ sleep(2.0)
93+
94+ self.launcher.add_launcher_item_from_position("Unity Test",
95+ "softwarecenter",
96+ 100,
97+ 100,
98+ 32,
99+ "/usr/share/applications/ubuntu-software-center.desktop",
100+ "")
101+
102+ sleep(2.0)
103+
104+ icon = self.launcher.model.get_icon_by_desktop_id("ubuntu-software-center.desktop")
105+
106+ # Check if the icon tooltip is NOT empty and NOT "waiting to install"
107+ self.assertThat(icon.tooltip_text == "Waiting to install", Equals(False))
108+ self.assertThat(len(icon.tooltip_text) == 0, Equals(False))
109+ sleep(.5)
110+
111 class LauncherDragIconsBehavior(LauncherTestCase):
112 """Tests interation with dragging icons with the Launcher"""
113
114@@ -525,7 +552,6 @@
115 get_launcher_icons_for_monitor(self.launcher_monitor)[-3]
116 self.assertThat(moved_icon.id, Equals(calc_icon.id))
117
118-
119 class LauncherRevealTests(LauncherTestCase):
120 """Test the launcher reveal behavior when in autohide mode."""
121

Subscribers

People subscribed via source and target branches