Merge lp:~bilalakhtar/unity/unhide-launcher-sc-fix-1002440 into lp:unity

Proposed by Bilal Akhtar on 2012-06-10
Status: Merged
Approved by: Brandon Schaefer on 2012-07-25
Approved revision: 2401
Merged at revision: 2519
Proposed branch: lp:~bilalakhtar/unity/unhide-launcher-sc-fix-1002440
Merge into: lp:unity
Diff against target: 15 lines (+5/-0)
1 file modified
launcher/LauncherController.cpp (+5/-0)
To merge this branch: bzr merge lp:~bilalakhtar/unity/unhide-launcher-sc-fix-1002440
Reviewer Review Type Date Requested Status
Brandon Schaefer (community) Approve on 2012-07-24
Marco Trevisan (Treviño) 2012-06-18 Approve on 2012-06-18
Tim Penhey (community) 2012-06-10 Needs Fixing on 2012-06-18
Review via email: mp+109501@code.launchpad.net

Commit message

The launcher will auto hide when installing a program from the software center now.

Description of the change

This branch fixes bug #1002440 by unhiding the launcher only when an animation can be completed successfully.

Merge request for the same bug fix to the Precise SRU branch is here: https://code.launchpad.net/~bilalakhtar/unity/5.0series-sru-software-center-integration-fixes

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

Would it not make sense to have an early return?

It seems that you are going to create a new launcher icon even if you are not going to show the launcher.
Is this what you expect?

Also,
  (!path.empty() && !path.compare("software-center-agent"))
is not very readable... how about
  (path == "software-centre=agent")

review: Needs Fixing
Bilal Akhtar (bilalakhtar) wrote :

I've made the change, thanks! Another review would be appreciated :)

Bilal Akhtar (bilalakhtar) wrote :

I've moved the SRU part to a common branch, and updated the description with clarification.

Tim Penhey (thumper) wrote :

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

This is just not easily understandable.

if (path != "software-center-agent")
    return

This does exactly the same thing, and doesn't require any boolean logic or double-checking the compare method.
Please change it.

review: Needs Fixing
Bilal Akhtar (bilalakhtar) wrote :

I've made the change, thanks!

Marco Trevisan (Treviño) (3v1n0) wrote :

+1

review: Approve
Didier Roche (didrocks) wrote :

Can we get a test for that before it's merged?

Bilal Akhtar (bilalakhtar) wrote :

On Mon, Jun 18, 2012 at 8:49 AM, Didier Roche <email address hidden> wrote:
> Can we get a test for that before it's merged?

It's a very minor change. I sure can do that for this Quantal branch
merge, but can someone please review the Precise SRU branch merge [1]?
It contains an SRU regression fix, and it needs to be SRUed ASAP,
since it's blocking a couple of bugs on Software Center.

[1]: https://code.launchpad.net/~bilalakhtar/unity/5.0series-sru-software-center-integration-fixes/+merge/110214

Unity Merger (unity-merger) wrote :

No commit message specified.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'launcher/LauncherController.cpp'
2--- launcher/LauncherController.cpp 2012-06-15 02:03:31 +0000
3+++ launcher/LauncherController.cpp 2012-06-18 03:21:26 +0000
4@@ -476,6 +476,11 @@
5 return;
6 }
7
8+ // Check if desktop file was supplied, or if it's set to SC's agent
9+ // See https://bugs.launchpad.net/unity/+bug/1002440
10+ if (path.empty() || path == "software-center-agent")
11+ return;
12+
13 SoftwareCenterLauncherIcon::Ptr result = CreateSCLauncherIcon(path, aptdaemon_trans_id, icon_path);
14
15 CurrentLauncher()->ForceReveal(true);