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

Proposed by Bilal Akhtar
Status: Merged
Approved by: Marco Trevisan (Treviño)
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
Brandon Schaefer (community) Approve
Thomi Richards (community) quality Approve
Tim Penhey (community) Needs Fixing
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.
Revision history for this message
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

Fixed string comparison, thanks Tim

Revision history for this message
Bilal Akhtar (bilalakhtar) wrote :

I've made the change, thanks!

Revision history for this message
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

Fix urgent quirk after installation of paid app

Revision history for this message
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.

Revision history for this message
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

Revision history for this message
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!!

Revision history for this message
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 :)

Revision history for this message
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!

Revision history for this message
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!

Revision history for this message
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.

Revision history for this message
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!

Revision history for this message
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

Added test, need to probably fix it though

2369. By Bilal Akhtar

Merge from 5.0 branch

Revision history for this message
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 :)

Revision history for this message
Thomi Richards (thomir-deactivatedaccount) 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)
Revision history for this message
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

Revision history for this message
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…

Revision history for this message
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!

Revision history for this message
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?

Revision history for this message
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! :)

Revision history for this message
Didier Roche (didrocks) wrote :

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

Revision history for this message
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!

Revision history for this message
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.

Revision history for this message
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

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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!

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

@Gary,

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

Revision history for this message
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.

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

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

review: Approve
Revision history for this message
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 :) )

Revision history for this message
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.

Revision history for this message
Ł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?

Revision history for this message
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

Revision history for this message
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.

Revision history for this message
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.
>

Revision history for this message
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