Merge lp:~bilalakhtar/unity/sc-integration-phase2 into lp:unity

[Bilal Akhtar]

This is the second phase of Software Center integration with the Unity launcher. The first phase included progress bars on the icon, as well as a "Waiting to install" tooltip. The second phase includes an icon moving animation from Software Center to the launcher, and also icon Activate() changes. The software center design spec for the whole feature is:

https://wiki.ubuntu.com/SoftwareCenter#Learning_how_to_launch_an_application

The first phase landed around Unity 5.2.0, but got broken right before the release of 5.2.0 (bug #932280). The fix to it lies in this branch: https://code.launchpad.net/~bilalakhtar/unity/fix-sc-launcher-integration/+merge/93908 . The fix has been waiting for review for more than 10 days now, can someone ensure that it gets in for Unity 5.6.0? Since that branch merely contains a fix (all strings have been in since before 5.2.0) so neither that branch nor this branch needs a UIFe.

Note that this branch contains the code from the above merge request too, so if someone were to merge this branch into lp:unity, there would be no need to merge lp:~bilalakhtar/unity/fix-sc-launcher-integration. The reason why I've made two different branches is because I'd like to give the lp:~bilalakhtar/unity/fix-sc-launcher-integration a higher priority (5.6.0) while this could get in to either 5.6.0 (unlikely) or 5.8.0.

The implementation is very stable, with unit tests and all.

Here's a YouTube video of this branch in action:
http://www.youtube.com/watch?v=6peOjOrcURA

[Bilal Akhtar (bilalakhtar)]

Bug request for the whole issue was bug #761851, but it got marked Fix Released after phase 1. Not sure if I should mark it Triaged again, or not.

[Didier Roche-Tolomelli (didrocks)]

This branche introduces a change in behavior (flying icon). It clearly needs an UIFe and a FFe.
Also, we saw that the previous code was broken without anyone noticing. This shows that you need tests for you code. Can you please first ensure that lp:~bilalakhtar/unity/fix-sc-launcher-integration gets autopilot tests for it not regressing again?

[Matthew Paul Thomas (mpt)]

Nice work! There are many details to tweak: the origin point is too high, the destination point looks to be on top of another launcher element rather than in a gap, the initial icon size is too small, and (as "krnekhelesh" pointed out on YouTube) the animation should happen after authentication rather than before. But this is a definite improvement, thank you.

review: Approve (design)

[Bilal Akhtar (bilalakhtar)]

@didrocks: When is Unity 5.6.0 coming out? Tomorrow, right?

I won't be able to add autopilot tests until today evening and that would mean a merge of lp:~bilalakhtar/unity/fix-sc-launcher-integration AFTER 5.6.0 (since by then everyone would have gone off to sleep). If that's the case, then can you merge that branch right now and I'll add tests today evening?

But if I'm wrong about the release date of 5.6.0 then don't merge that branch until I've added tests.

@mpt: Thanks for the review! Since you've approved already I'll ensure this gets in before I work on those tweaks.

[Didier Roche-Tolomelli (didrocks)]

Hey Bilal, there are two things:
5.6 is coming out tomorrow morning if the last regression is fixed. We don't merge anything for a week already as we are in freeze. So this would mean your branch will be merged for 5.8 (once you unfreeze).

Oh, and we don't let entering trunk code without test, so yeah, it's a pre-requisite to push to the other branch some tests.

For that one, you will need to have a Feature freeze exception acked and a UI Freeze exception acked as well before meriging in trunk (in addition to test and from a review from some dx guys, I'm not doing the review. ;)).

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

64 + sigc::signal<void, std::string const&, AbstractLauncherIcon::Ptr, std::string const&, std::string const&,
65 + gint32, gint32, gint32> launcher_addrequest_special;

You can just use int at this level, so we are more C++ conformant.

139 + ((SoftwareCenterLauncherIcon*)result.GetPointer())->Animate(launcher_, icon_x, icon_y, icon_size);

Don't use C-style casts.

184 + finished = true;
185 + finished_just_now = true;

Set them using the constructor initializer list.

201 +void SoftwareCenterLauncherIcon::AddSelfToLauncher()
202 +{
203 + _launcher->icon_animation_complete.emit(self_abstract);
204 +}

306 + void AddSelfToLauncher();

322 + AbstractLauncherIcon::Ptr self_abstract;

I don't really think you need those, please don't do that. You already handling an AbstractLauncherIcon.

244 + target_x = (int)current_bamf_icon->GetCenter(launcher->monitor).x;

Again, use static_cast

256 + g_timeout_add(30, &SoftwareCenterLauncherIcon::OnDragWindowAnimComplete, this);

Maybe it can be a little more lazy.
Anyway I'd focus on fixing the problem at the source, instead of using this workaround.

308 + static gboolean OnDragWindowAnimComplete(gpointer data);

Put in private fields.

310 + void Animate(nux::ObjectPtr<Launcher> launcher, gint32 icon_x, gint32 icon_y, gint32 icon_size);

You can also just use Animate(nux::ObjectPtr<Launcher> const& launcher ... And move to pure int's.

312 + void ActivateLauncherIcon(ActionArg arg);

Put it in protected field.

323 + bool finished;
324 + bool finished_just_now;

Add the _ as prefix for private variables. (Actually it would be betteer use a suffixed _, as defined by our coding standards, but in that case you need to update all the members).

For testing, I think you can also emulate something without autopilot by making your LauncherSpecial request to be caused by a fake dbus server you write on tests.

review: Needs Fixing

[Bilal Akhtar (bilalakhtar)]

I've made all the changes that were possible as per Marco's review, except for the tests (which I'm adding now) and I also left some other ones which would break the code if I did change (I've left code comments at those spots).

[Gary Lasker (gary-lasker)]

Hi Bilal! I just want to say thank you for this. It's beautiful!! :D

I am very much looking forward to seeing this in Precise!!!

Thanks everybody,
Gary

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

189 + _finished = true;
190 + _finished_just_now = true;

You don't need them anymore.

329 + AbstractLauncherIcon::Ptr self_abstract;

Are you sure you need to save that on the class? I guess you can just Reference your icon before that it gets unreferenced...

+ if (((int)current_bamf_icon->GetCenter(launcher->monitor).x) != 0 &&
248 + ((int)current_bamf_icon->GetCenter(launcher->monitor).y) != 0)

Those are still C-style casts :)

139 + static_cast<SoftwareCenterLauncherIcon*>(result.GetPointer())->Animate(launcher_, icon_x, icon_y, icon_size);

Do this only if you're sure that this won't happen for another kind of icon... Otherwise use, at least, the dynamic_cast.

[Bilal Akhtar (bilalakhtar)]

Branch is ready again, please review :)

[lp:~bilalakhtar/unity/sc-integration-phase2]

2013. By Bilal Akhtar on 2012-03-09

Change C-style casts

[Bilal Akhtar (bilalakhtar)]

> 189 + _finished = true;
> 190 + _finished_just_now = true;
>
> You don't need them anymore.

Those lines are part of the Finished function, so they're very much needed for the Activate function to work.

>
> 329 + AbstractLauncherIcon::Ptr self_abstract;
>
> Are you sure you need to save that on the class? I guess you can just
> Reference your icon before that it gets unreferenced...
>

That's the problem. I did try that, but it didn't work. Got a segfault (totally strange). Storing it's own reference in a variable worked, though.

> + if (((int)current_bamf_icon->GetCenter(launcher->monitor).x) != 0 &&
> 248 + ((int)current_bamf_icon->GetCenter(launcher->monitor).y) !=
> 0)
>
> Those are still C-style casts :)

Fixed in latest commit.

>
> 139 + static_cast<SoftwareCenterLauncherIcon*>(result.GetPointer())->Anim
> ate(launcher_, icon_x, icon_y, icon_size);
>
> Do this only if you're sure that this won't happen for another kind of icon...
> Otherwise use, at least, the dynamic_cast.

Yes, it won't happen, because I'm using a special function to create the result variable. That function is hard-coded to create only a SoftwareCenterLauncherIcon and nothing else. So a static cast is safe here.

Thanks for the review anyway, it would be great if you reviewed again!

[lp:~bilalakhtar/unity/sc-integration-phase2]

2014. By Bilal Akhtar on 2012-03-09

Fix a line in tests

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

> Those lines are part of the Finished function, so they're very much needed for
> the Activate function to work.

Oh, yes sure. I didn't notice they were inside the lambda, sorry.

> > 329 + AbstractLauncherIcon::Ptr self_abstract;
> >
> > Are you sure you need to save that on the class? I guess you can just
> > Reference your icon before that it gets unreferenced...
> >
>
> That's the problem. I did try that, but it didn't work. Got a segfault
> (totally strange). Storing it's own reference in a variable worked, though.

I want to look to this further and do some experiments with it... I think we should find a cleaner solution.

357 +class SoftwareCenterLauncherIcon(SimpleLauncherIcon):
358 + """Represents a launcher icon of a Software Center app."""

Also on tests SoftwareCenterLauncherIcon should extend BamfLauncherIcon.
This would also avoid the change on line 378

About the autoplito tests, I think that maybe you should define functions to add icons to the launcher in the emulator, also once you've finished them ask a review by Thomi for the tests.

[Bilal Akhtar (bilalakhtar)]

I myself have given up on the AbstractLauncherIcon::Ptr thing after multiple hits and misses. I think it's okay in it's current form, an extra line or two but not too ugly.

Other than that, do you think the code (other than the tests) is good? Could you approve the code then? Of course I'd need to file a UIFe and FFe request before it can get merged, but in the meantime you can approve/reject it.

[Thomi Richards (thomir-deactivatedaccount)]

Hi,

There's a problem with the autopilot test (Exception on cleanup), which I will look at and fix on Monday. However, more concerning is that running the autopilot test causes unity / compiz to crash frequently (maybe 50% of the time). I will attempt to look into this as well, and let you know what the issue is.

@Marco - you mentioned above that it might be a good idea to add methods to the Launcher emulator for adding and removing icons - we already have this, but we can't use it in this instance, since this isn't a regular application icon.

review: Needs Fixing

[Thomi Richards (thomir-deactivatedaccount)]

Hi,

Upon further investigation, the crash is inside the software center launcher icon. I get a segfault about 50% of the time when running the autopilot test. Here's the stack trace:

#0 0x00007fffe4f02ca2 in unity::launcher::SoftwareCenterLauncherIcon::Animate (this=0x0, launcher=..., icon_x=100, icon_y=100, icon_size=32)
    at /home/thomi/code/canonical/unity/sc-integration-phase2/plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp:92
#1 0x00007fffe509ecd7 in unity::launcher::Controller::Impl::OnLauncherAddRequestSpecial (this=0x903c90, path=..., before=..., aptdaemon_trans_id=..., icon_path=...,
    icon_x=100, icon_y=100, icon_size=32) at /home/thomi/code/canonical/unity/sc-integration-phase2/plugins/unityshell/src/LauncherController.cpp:406
#2 0x00007fffe50aa989 in sigc::bound_mem_functor7<void, unity::launcher::Controller::Impl, std::string const&, nux::ObjectPtr<unity::launcher::AbstractLauncherIcon>, std::string const&, std::string const&, int, int, int>::operator() (this=0xd24498, _A_a1=..., _A_a2=..., _A_a3=..., _A_a4=..., _A_a5=@0x7fffffffe0f0: 100,
    _A_a6=@0x7fffffffe0f4: 100, _A_a7=@0x7fffffffe0f8: 32) at /usr/include/sigc++-2.0/sigc++/functors/mem_fun.h:2277
#3 0x00007fffe50a9d5f in sigc::adaptor_functor<sigc::bound_mem_functor7<void, unity::launcher::Controller::Impl, std::string const&, nux::ObjectPtr<unity::launcher::AbstractLauncherIcon>, std::string const&, std::string const&, int, int, int> >::operator()<std::string const&, nux::ObjectPtr<unity::launcher::AbstractLauncherIcon> const&, std::string const&, std::string const&, int const&, int const&, int const&> (this=0xd24490, _A_arg1=..., _A_arg2=..., _A_arg3=..., _A_arg4=...,
    _A_arg5=@0x7fffffffe0f0: 100, _A_arg6=@0x7fffffffe0f4: 100, _A_arg7=@0x7fffffffe0f8: 32) at /usr/include/sigc++-2.0/sigc++/adaptors/adaptor_trait.h:213
#4 0x00007fffe50a89b2 in sigc::internal::slot_call7<sigc::bound_mem_functor7<void, unity::launcher::Controller::Impl, std::string const&, nux::ObjectPtr<unity::launcher::AbstractLauncherIcon>, std::string const&, std::string const&, int, int, int>, void, std::string const&, nux::ObjectPtr<unity::launcher::AbstractLauncherIcon>, std::string const&, std::string const&, int, int, int>::call_it (rep=0xd24460, a_1=..., a_2=..., a_3=..., a_4=..., a_5=@0x7fffffffe0f0: 100, a_6=@0x7fffffffe0f4: 100,
    a_7=@0x7fffffffe0f8: 32) at /usr/include/sigc++-2.0/sigc++/functors/slot.h:383
#5 0x00007fffe508b1e9 in sigc::internal::signal_emit7<void, std::string const&, nux::ObjectPtr<unity::launcher::AbstractLauncherIcon>, std::string const&, std::string const&, int, int, int, sigc::nil>::emit (impl=0xd24410, _A_a1=..., _A_a2=..., _A_a3=..., _A_a4=..., _A_a5=@0x7fffffffe0f0: 100, _A_a6=@0x7fffffffe0f4: 100,
    _A_a7=@0x7fffffffe0f8: 32) at /usr/include/sigc++-2.0/sigc++/signal.h:2567
#6 0x00007fffe508995d in sigc::signal7<void, std::string const&, nux::ObjectPtr<unity::launcher::AbstractLauncherIcon>, std::string const&, std::string const&, int, int, int, sigc::nil>::emit (this=0xca1690, _A_a1=..., _A_a2=..., _A_a3=..., _A_a4=..., _A_a5=@0x7fffffffe0f0: 100, _A_a6=@0x7fffffffe0f4: 100, _A_a7=@0x7fffffffe0f8: 32)
    at /usr/include/sigc++-2.0/sigc++/s...

review: Needs Fixing

[lp:~bilalakhtar/unity/sc-integration-phase2]

2015. By Bilal Akhtar on 2012-03-13

Fix crash on second install, THANKS THOMI

[Bilal Akhtar (bilalakhtar)]

Thomi:

I've fixed the crash, pull the branch and you won't have the problem again :)

[Bilal Akhtar (bilalakhtar)]

Also, Marco: Can you review the code? Everything's final now, Thomi will be reviewing the tests. The code is final and works perfectly now.

[lp:~bilalakhtar/unity/sc-integration-phase2]

2016. By Bilal Akhtar on 2012-03-13

Merge from trunk, fix conflicts

[Alex Launi (alexlauni)]

I don't like how the test skips if the software center is already pinned. You should have your setup ensure that the launcher is not pinned, so that the test can execute.

review: Needs Fixing (test)

[lp:~bilalakhtar/unity/sc-integration-phase2]

2017. By Bilal Akhtar on 2012-03-13

Cleanup tests, also first ensure Software Center is removed

[Bilal Akhtar (bilalakhtar)]

Thanks for the review Alex, now it unlocks the icon before proceeding. That fixes the issue. Can you review the tests again now?

[Didier Roche-Tolomelli (didrocks)]

Note that it will need a Feature Freeze and UI Freeze exception acked. Can you please proceed it?

[Bilal Akhtar (bilalakhtar)]

I thought that was after the code review? Anyway, I'll do that now.

[Didier Roche-Tolomelli (didrocks)]

that's to be done in parallel in fact as you need to get the release team approval before the code is merged.

[Bilal Akhtar (bilalakhtar)]

I've filed bug #955147 , and subscribed ubuntu-release to it. That bug contains everything needed for the exception requests. I've also posted to the docs and translators mailing lists and linked the postings there.

[Alex Launi (alexlauni)]

The dbus code should be moved into the launcher emulator. Secondly, the assertion that there are more icons may not be true. Consider the case where some applications crash unexpectedly. I would just check for the waiting to install tooltip. That's the real sign that everything worked.

I don't think this test needs to go in its own class. It will be just fine in LauncherTests. I also feel like this test might need some clean up routine to return the desktop to its original state. Does the AddLauncherItemFromPosition call pin the launcher, or is it transient?

review: Needs Fixing

[lp:~bilalakhtar/unity/sc-integration-phase2]

2018. By Bilal Akhtar on 2012-03-14

Make changes in tests, merge function into LauncherTests, etc

2019. By Bilal Akhtar on 2012-03-14

Cleanup at the end, move function to the proper class

[Alex Launi (alexlauni)]

emulate_software_center_dbus_call is sort of a bad name, this might get used somewhere else so just wrap the launcher function and call it add_launcher_itom_from_position.

instead of importing dbus and doing bus.SessionBus just do:
   from autopilot.emulators.dbus_handler import session_bus
session_bus is your bus object.

define the desktop file in a variable instead of reusing the string over and over.

sleep should not be the first thing you do in your test, also not the last!

You need to check that the icon isn't None when you get it after the dbus call. If the call failed, then the test will crash. So assert that it is not None
self.assertThat(icon, Not(Is(None)))

Instead of asserting that icon_text == "blah", Equals(True) do this
self.assertThat(icon[0].tooltip_text, Equals("Waiting to install"))

cleanup should go like this,
add an anonymous function to your test
def cleanup():
    if icon is not None:
        launcher_instance.unlock_from_launcher(icon[0])
then
icon = self.launcher.model.get_icon_by_desktop_file(DESKTOP_FILE_PATH)
if icon is not None:
    launcher_instance.unlock_from_launcher(icon[0])
    sleep(1)
else:
    self.addCleanup(cleanup)

review: Needs Fixing

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

Also, make sure to cover in your test this change (it should be implicit, I guess): https://code.launchpad.net/~bilalakhtar/unity/fix-sc-launcher-integration/+merge/93908 so we can merge all together (if we get the exception, otherwise you need to backport the tests to only that branch).

[Bilal Akhtar (bilalakhtar)]

  Hi Marco,

That change is already covered. In fact, the whole test function evolves
around that.

There would be no way to make it implicit without going through the whole
procedure,since all functions are interconnected.

I'll make the rest if the changes soon.

------------------------------
*From:* "Marco Trevisan (Treviño)" <[e-mail]>
*To:* "Bilal Akhtar" <email address hidden>
*Sent:* 14 March, 2012 1:44 PM
*Subject:* Re: [Merge] lp:~bilalakhtar/unity/sc-integration-phase2 into
lp:unity

Also, make sure to cover in your test this change (it should be implicit, I
guess):
https://code.launchpad.net/~bilalakhtar/unity/fix-sc-launcher-integration/+merge/93908so
we can merge all together (if we get the exception, otherwise you need
to backport the tests to only that branch).
--
https://code.launchpad.net/~bilalakhtar/unity/sc-integration-phase2/+merge/95795
You are the owner of lp:~bilalakhtar/unity/sc-integration-phase2.

[lp:~bilalakhtar/unity/sc-integration-phase2]

2020. By Bilal Akhtar on 2012-03-14

Cleanup tests even further, follow Alex's recommendations, thanks a lot Alex

2021. By Bilal Akhtar on 2012-03-14

Merge from trunk

[Alex Launi (alexlauni)]

Tests now look good Bilal

review: Approve (testing)

[Alex Launi (alexlauni)]

I missed this yesterday, please move def cleanup() to be the first thing in test_software_center_add_icon

review: Needs Fixing

[lp:~bilalakhtar/unity/sc-integration-phase2]

2022. By Bilal Akhtar on 2012-03-15

Move definition of cleanup function - thanks Alex

2023. By Bilal Akhtar on 2012-03-15

Merge from trunk

[Alex Launi (alexlauni)]

Aced

review: Approve

=== modified file 'plugins/unityshell/src/BamfLauncherIcon.h'
--- plugins/unityshell/src/BamfLauncherIcon.h	2012-02-12 10:43:11 +0000
+++ plugins/unityshell/src/BamfLauncherIcon.h	2012-03-15 17:05:24 +0000
@@ -43,7 +43,7 @@
   BamfLauncherIcon(BamfApplication* app);
   virtual ~BamfLauncherIcon();
 
-  void ActivateLauncherIcon(ActionArg arg);
+  virtual void ActivateLauncherIcon(ActionArg arg);
 
   std::string DesktopFile();
 
 
=== modified file 'plugins/unityshell/src/Launcher.cpp'
--- plugins/unityshell/src/Launcher.cpp	2012-03-13 17:20:30 +0000
+++ plugins/unityshell/src/Launcher.cpp	2012-03-15 17:05:24 +0000
@@ -1720,6 +1720,11 @@
   Resize();
 }
 
+int Launcher::GetIconSize()
+{
+    return _icon_size;
+}
+
 void Launcher::Resize()
 {
   UScreen* uscreen = UScreen::GetDefault();
@@ -2877,7 +2882,8 @@
     g_variant_get(parameters, "(ssiiiss)", &title, &icon, &icon_x, &icon_y, &icon_size, &desktop_file, &aptdaemon_task, NULL);
 
     Launcher* self = (Launcher*)user_data;
-    self->launcher_addrequest_special.emit(desktop_file, AbstractLauncherIcon::Ptr(), aptdaemon_task, icon);
+    self->launcher_addrequest_special.emit(desktop_file, AbstractLauncherIcon::Ptr(), aptdaemon_task, icon,
+                                            icon_x, icon_y, icon_size);
 
     g_dbus_method_invocation_return_value(invocation, nullptr);
     g_free(icon);
 
=== modified file 'plugins/unityshell/src/Launcher.h'
--- plugins/unityshell/src/Launcher.h	2012-03-13 02:07:54 +0000
+++ plugins/unityshell/src/Launcher.h	2012-03-15 17:05:24 +0000
@@ -40,6 +40,7 @@
 #include "LauncherHideMachine.h"
 #include "LauncherHoverMachine.h"
 #include "UBusWrapper.h"
+#include "SoftwareCenterLauncherIcon.h"
 
 
 namespace unity
@@ -68,6 +69,7 @@
   AbstractLauncherIcon::Ptr GetSelectedMenuIcon() const;
 
   void SetIconSize(int tile_size, int icon_size);
+  int GetIconSize();
 
   LauncherHideMachine* HideMachine() { return _hide_machine; }
 
@@ -113,8 +115,10 @@
   void Resize();
 
   sigc::signal<void, char*, AbstractLauncherIcon::Ptr> launcher_addrequest;
-  sigc::signal<void, std::string const&, AbstractLauncherIcon::Ptr, std::string const&, std::string const&> launcher_addrequest_special;
+  sigc::signal<void, std::string const&, AbstractLauncherIcon::Ptr, std::string const&, std::string const&,
+               int, int, int> launcher_addrequest_special;
   sigc::signal<void, AbstractLauncherIcon::Ptr> launcher_removerequest;
+  sigc::signal<void, AbstractLauncherIcon::Ptr> icon_animation_complete;
   sigc::signal<void> selection_change;
   sigc::signal<void> hidden_changed;
 
@@ -128,6 +132,8 @@
 
   static const int ANIM_DURATION_SHORT;
 
+  void RenderIconToTexture(nux::GraphicsEngine& GfxContext, AbstractLauncherIcon::Ptr icon, nux::ObjectPtr<nux::IOpenGLBaseTexture> texture);
+
 protected:
   // Introspectable methods
   std::string GetName() const;
@@ -278,8 +284,6 @@
 
   void OnActionDone(GVariant* data);
 
-  void RenderIconToTexture(nux::GraphicsEngine& GfxContext, AbstractLauncherIcon::Ptr icon, nux::ObjectPtr<nux::IOpenGLBaseTexture> texture);
-
   AbstractLauncherIcon::Ptr MouseIconIntersection(int x, int y);
   void EventLogic();
   void MouseDownLogic(int x, int y, unsigned long button_flags, unsigned long key_flags);
 
=== modified file 'plugins/unityshell/src/LauncherController.cpp'
--- plugins/unityshell/src/LauncherController.cpp	2012-03-14 23:22:02 +0000
+++ plugins/unityshell/src/LauncherController.cpp	2012-03-15 17:05:24 +0000
@@ -86,9 +86,12 @@
   void OnIconRemoved(AbstractLauncherIcon::Ptr icon);
 
   void OnLauncherAddRequest(char* path, AbstractLauncherIcon::Ptr before);
-  void OnLauncherAddRequestSpecial(std::string const& path, AbstractLauncherIcon::Ptr before, std::string const& aptdaemon_trans_id, std::string const& icon_path);
+  void OnLauncherAddRequestSpecial(std::string const& path, AbstractLauncherIcon::Ptr before, std::string const& aptdaemon_trans_id, std::string const& icon_path,
+                                   int icon_x, int icon_y, int icon_size);
   void OnLauncherRemoveRequest(AbstractLauncherIcon::Ptr icon);
 
+  void OnSCIconAnimationComplete(AbstractLauncherIcon::Ptr icon);
+
   void OnLauncherEntryRemoteAdded(LauncherEntryRemote* entry);
   void OnLauncherEntryRemoteRemoved(LauncherEntryRemote* entry);
 
@@ -340,6 +343,8 @@
   launcher->launcher_addrequest_special.connect(sigc::mem_fun(this, &Impl::OnLauncherAddRequestSpecial));
   launcher->launcher_removerequest.connect(sigc::mem_fun(this, &Impl::OnLauncherRemoveRequest));
 
+  launcher->icon_animation_complete.connect(sigc::mem_fun(this, &Impl::OnSCIconAnimationComplete));
+
   parent_->AddChild(launcher);
 
   return launcher;
@@ -393,7 +398,10 @@
 Controller::Impl::OnLauncherAddRequestSpecial(std::string const& path,
                                               AbstractLauncherIcon::Ptr before,
                                               std::string const& aptdaemon_trans_id,
-                                              std::string const& icon_path)
+                                              std::string const& icon_path,
+                                              int icon_x,
+                                              int icon_y,
+                                              int icon_size)
 {
   auto launchers = model_->GetSublist<BamfLauncherIcon>();
   for (auto icon : launchers)
@@ -403,14 +411,24 @@
   }
 
   AbstractLauncherIcon::Ptr result = CreateSCLauncherIcon(path, aptdaemon_trans_id, icon_path);
+  
+  launcher_->ForceReveal(true);
+
   if (result)
+    static_cast<SoftwareCenterLauncherIcon*>(result.GetPointer())->Animate(launcher_, icon_x, icon_y, icon_size);
+}
+
+void Controller::Impl::OnSCIconAnimationComplete(AbstractLauncherIcon::Ptr icon)
+{
+
+  if (icon)
   {
-    RegisterIcon(result);
-
-    if (before)
-      model_->ReorderBefore(result, before, false);
+    RegisterIcon(icon);
   }
   Save();
+
+  launcher_->ForceReveal(false);
+  
 }
 
 void Controller::Impl::SortAndUpdate()
@@ -714,8 +732,6 @@
     return result;
   }
 
-  g_object_set_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen"), GINT_TO_POINTER(1));
-
   bamf_view_set_sticky(BAMF_VIEW(app), true);
   AbstractLauncherIcon::Ptr icon(new SoftwareCenterLauncherIcon(app, aptdaemon_trans_id, icon_path));
   icon->SetSortPriority(sort_priority_++);
 
=== modified file 'plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp'
--- plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp	2012-02-12 10:43:11 +0000
+++ plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp	2012-03-15 17:05:24 +0000
@@ -20,6 +20,8 @@
 
 #include <glib/gi18n-lib.h>
 #include "SoftwareCenterLauncherIcon.h"
+#include "Launcher.h"
+#include "LauncherModel.h"
 
 namespace unity
 {
@@ -34,19 +36,108 @@
                    aptdaemon_trans_id,
                    "org.debian.apt.transaction",
                    G_BUS_TYPE_SYSTEM,
-                   G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START)
+                   G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START),
+  _finished (true),
+  _finished_just_now (false)
 {
+
   _aptdaemon_trans.Connect("PropertyChanged", sigc::mem_fun(this, &SoftwareCenterLauncherIcon::OnPropertyChanged));
   _aptdaemon_trans.Connect("Finished", [&] (GVariant *) {
     tooltip_text = BamfName();
     SetQuirk(QUIRK_PROGRESS, false);
+    SetQuirk(QUIRK_URGENT, true);
     SetProgress(0.0f);
+    _finished = true;
+    _finished_just_now = true;
   });
 
   SetIconType(TYPE_APPLICATION);
   icon_name = icon_path.c_str();
   tooltip_text = _("Waiting to install");
-}
+
+  // For no clear reason, Unity segfaults if we try to generate a pointer of "this" twice.
+  // Hence, just generate it once and store it.
+  self_abstract = AbstractLauncherIcon::Ptr(this);
+  
+}
+
+void SoftwareCenterLauncherIcon::AddSelfToLauncher()
+{
+    _launcher->icon_animation_complete.emit(self_abstract);
+}
+
+gboolean SoftwareCenterLauncherIcon::OnDragWindowAnimComplete(gpointer data)
+{
+    SoftwareCenterLauncherIcon* self = (SoftwareCenterLauncherIcon*) data;
+    if (!self->_drag_window->Animating()) {
+        self->_drag_window->ShowWindow(false);
+
+        // I had to create a new function because for some reason, when trying to access
+        // self->_launcher from this function, I got segfaults.
+        self->AddSelfToLauncher();
+        return false;
+    }
+    return true;
+}
+
+void
+SoftwareCenterLauncherIcon::Animate(nux::ObjectPtr<Launcher> launcher,
+                                    int icon_x,
+                                    int icon_y,
+                                    int icon_size)
+{
+  int target_x = 0;
+  int target_y = 0;
+
+  _launcher = launcher.GetPointer();
+    
+  _icon_texture = nux::GetGraphicsDisplay()->GetGpuDevice()->CreateSystemCapableDeviceTexture(launcher->GetIconSize(), launcher->GetIconSize(), 1, nux::BITFMT_R8G8B8A8);
+  _drag_window = new LauncherDragWindow(_icon_texture);
+
+  launcher->RenderIconToTexture(nux::GetWindowThread()->GetGraphicsEngine(), self_abstract, _icon_texture);
+  nux::Geometry geo = _drag_window->GetGeometry();
+  _drag_window->SetBaseXY(icon_x, icon_y);
+  _drag_window->ShowWindow(true);
+  _drag_window->SinkReference();
+
+  // Find out the center of last BamfLauncherIcon with non-zero co-ordinates
+  auto launchers = launcher->GetModel()->GetSublist<BamfLauncherIcon>();
+  for (auto current_bamf_icon : launchers)
+  {
+    if (static_cast<int>(current_bamf_icon->GetCenter(launcher->monitor).x) != 0 &&
+        static_cast<int>(current_bamf_icon->GetCenter(launcher->monitor).y) != 0)
+    {
+       target_x = static_cast<int>(current_bamf_icon->GetCenter(launcher->monitor).x);
+       target_y = static_cast<int>(current_bamf_icon->GetCenter(launcher->monitor).y);
+    }
+  }
+
+  target_y = target_y + (launcher->GetIconSize() / 2);
+  _drag_window->SetAnimationTarget(target_x, target_y);
+
+  _drag_window->StartAnimation();
+
+  // This is to check if the animation has completed, since the anim_completed signal
+  // in the drag window doesn't work (mysteriously)
+  g_timeout_add(30, &SoftwareCenterLauncherIcon::OnDragWindowAnimComplete, this);
+
+}
+
+void SoftwareCenterLauncherIcon::ActivateLauncherIcon(ActionArg arg)
+{
+    if (_finished)
+    {
+        if (_finished_just_now)
+        {
+            SetQuirk(QUIRK_URGENT, false);
+            _finished_just_now = false;
+        }
+        BamfLauncherIcon::ActivateLauncherIcon(arg);
+    }
+    else
+        SetQuirk(QUIRK_STARTING, false);
+}
+
 
 void
 SoftwareCenterLauncherIcon::OnPropertyChanged(GVariant* params)
@@ -63,7 +154,10 @@
     g_variant_get(property_value, "i", &progress);
 
     if (progress < 100)
+    {
       SetQuirk(QUIRK_PROGRESS, true);
+      _finished = false;
+    }
 
     SetProgress(progress/100.0f);
   }
@@ -71,5 +165,10 @@
   g_variant_unref(property_value);
 }
 
+std::string SoftwareCenterLauncherIcon::GetName() const
+{
+    return "SoftwareCenterLauncherIcon";
+}
+
 }
 }
 
=== modified file 'plugins/unityshell/src/SoftwareCenterLauncherIcon.h'
--- plugins/unityshell/src/SoftwareCenterLauncherIcon.h	2012-02-03 01:24:53 +0000
+++ plugins/unityshell/src/SoftwareCenterLauncherIcon.h	2012-03-15 17:05:24 +0000
@@ -22,6 +22,8 @@
 #define SOFTWARE_CENTER_LAUNCHERICON_H
 
 #include "BamfLauncherIcon.h"
+#include "LauncherDragWindow.h"
+#include "Launcher.h"
 #include <UnityCore/GLibDBusProxy.h>
 
 namespace unity
@@ -36,10 +38,28 @@
                              std::string const& aptdaemon_trans_id,
                              std::string const& icon_path);
 
+  void AddSelfToLauncher();
+
+  void Animate(nux::ObjectPtr<Launcher> launcher, int icon_x, int icon_y, int icon_size);
+
+  std::string GetName() const;
+
+protected:
+  void ActivateLauncherIcon(ActionArg arg);
+
 private:
   void OnPropertyChanged(GVariant* params);
 
+  static gboolean OnDragWindowAnimComplete(gpointer data);
+
   glib::DBusProxy _aptdaemon_trans;
+
+  nux::ObjectPtr<nux::IOpenGLBaseTexture> _icon_texture;
+  LauncherDragWindow* _drag_window;
+  Launcher* _launcher;
+  AbstractLauncherIcon::Ptr self_abstract;
+  bool _finished;
+  bool _finished_just_now;
 };
 
 }
 
=== modified file 'tests/autopilot/autopilot/emulators/unity/icons.py'
--- tests/autopilot/autopilot/emulators/unity/icons.py	2012-03-04 23:12:19 +0000
+++ tests/autopilot/autopilot/emulators/unity/icons.py	2012-03-15 17:05:24 +0000
@@ -48,3 +48,6 @@
 
 class DesktopLauncherIcon(SimpleLauncherIcon):
     """Represents an icon that may appear in the switcher."""
+
+class SoftwareCenterLauncherIcon(BamfLauncherIcon):
+    """Represents a launcher icon of a Software Center app."""
 
=== modified file 'tests/autopilot/autopilot/emulators/unity/launcher.py'
--- tests/autopilot/autopilot/emulators/unity/launcher.py	2012-03-13 20:44:08 +0000
+++ tests/autopilot/autopilot/emulators/unity/launcher.py	2012-03-15 17:05:24 +0000
@@ -7,6 +7,7 @@
 # by the Free Software Foundation.
 #
 
+import dbus
 import logging
 from time import sleep
 
@@ -14,7 +15,7 @@
 from autopilot.emulators.unity import UnityIntrospectionObject
 from autopilot.emulators.unity.icons import BamfLauncherIcon, SimpleLauncherIcon
 from autopilot.emulators.X11 import Mouse, ScreenGeometry
-
+from autopilot.emulators.dbus_handler import session_bus
 
 logger = logging.getLogger(__name__)
 
@@ -37,6 +38,18 @@
     def key_nav_monitor(self):
         return self.key_nav_launcher_monitor
 
+    def add_launcher_item_from_position(self,name,icon,icon_x,icon_y,icon_size,desktop_file,aptdaemon_task):
+        """ Emulate a DBus call from Software Center to pin an icon to the launcher """
+        launcher_object = session_bus.get_object('com.canonical.Unity.Launcher',
+                                      '/com/canonical/Unity/Launcher')
+        launcher_iface = dbus.Interface(launcher_object, 'com.canonical.Unity.Launcher')
+        launcher_iface.AddLauncherItemFromPosition(name,
+                                                   icon,
+                                                   icon_x,
+                                                   icon_y,
+                                                   icon_size,
+                                                   desktop_file,
+                                                   aptdaemon_task)
 
 class Launcher(UnityIntrospectionObject, KeybindingsHelper):
     """An individual launcher for a monitor."""
 
=== modified file 'tests/autopilot/autopilot/tests/test_launcher.py'
--- tests/autopilot/autopilot/tests/test_launcher.py	2012-03-14 01:58:37 +0000
+++ tests/autopilot/autopilot/tests/test_launcher.py	2012-03-15 17:05:24 +0000
@@ -8,7 +8,7 @@
 # by the Free Software Foundation.
 
 import logging
-from testtools.matchers import Equals, NotEquals, LessThan, GreaterThan
+from testtools.matchers import Equals, NotEquals, LessThan, GreaterThan, Not, Is
 from time import sleep
 
 from autopilot.tests import AutopilotTestCase
@@ -395,6 +395,41 @@
         self.assertThat(self.launcher.key_nav_is_active, Equals(True))
         self.assertThat(self.launcher.key_nav_is_grabbed, Equals(True))
 
+    def test_software_center_add_icon(self):
+        """ Test the ability to add a SoftwareCenterLauncherIcon """
+        
+        def cleanup():
+            if icon is not None:
+                launcher_instance.unlock_from_launcher(icon[0])
+
+        launcher_instance = self.get_launcher()
+        sc_desktop_file = "/usr/share/applications/ubuntu-software-center.desktop"
+                
+        # Check if SC is pinned to the launcher already
+        icon = self.launcher.model.get_icon_by_desktop_file(sc_desktop_file)
+        if icon != None:
+            launcher_instance.unlock_from_launcher(icon[0])
+            sleep(2.0) # Animation of removing icon can take over a second
+        else:
+            self.addCleanup(cleanup)
+
+        self.launcher.add_launcher_item_from_position("Unity Test",
+                                                   "softwarecenter",
+                                                   100,
+                                                   100,
+                                                   32,
+                                                   sc_desktop_file,
+                                                   "")
+        
+        sleep(1.0)
+
+        icon = self.launcher.model.get_icon_by_desktop_file(sc_desktop_file)
+        self.assertThat(icon, Not(Is(None)))
+
+        # Check for whether:
+        # The new launcher icon has a 'Waiting to install' tooltip
+        self.assertThat(icon[0].tooltip_text, Equals("Waiting to install"))
+
 class LauncherRevealTests(ScenariodLauncherTests):
     """Test the launcher reveal bahavior when in autohide mode."""