Merge lp:~thomir-deactivatedaccount/unity/sc-integration-phase2 into lp:unity

[Tim Penhey]

Add animation of icon moving to the launcher, and the wiggle when installation is complete.

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

[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!

[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

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

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

[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

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

[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

[Alex Launi (alexlauni)]

Aced

review: Approve

[Tim Penhey (thumper)]

There were a number of C++ fixes needed. Quicker to just take over at this point to make sure we get this landed. Thanks Bilal for all your work on this.

[Tim Penhey (thumper)]

Code is good, has AP test.

review: Approve

[Alex Launi (alexlauni)]

Can you update the autopilot test to move the cleanup routine to the beginning of the test method.

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

 + //TODO: don't iterate through them and pick the last one, just use back() to get the last one.
375 + for (auto current_bamf_icon : bamf_icons)
376 + {

Wouldn't be better to use a reverse iterator at this point, until the back()?

31 + _drag_window->SetAnimationTarget((int)(_drag_icon->GetCenter(monitor).x),
32 (int)(_drag_icon->GetCenter(monitor).y));

There are still some C casts, move them away when finding them :)

157 + void OnLauncherAddRequestSpecial(std::string const& path, AbstractLauncherIcon::Ptr before, std::string const& aptdaemon_trans_id, std::string const& icon_path,
158 + int icon_x, int icon_y, int icon_size);

Use one line, or split it to fit the classic 80th coulumn ;)

428 SetProgress(progress/100.0f);
429 + g_variant_unref(property_value);

glib::Variant here could be more friendly ;)

[Bilal Akhtar (bilalakhtar)]

Thanks for the review Marco, I think we'll go ahead with how it is right now, since I don't want any more delays (so that we don't miss the 5.8 release for this change) and I'll make all those changes you suggested coupled with a few tweaks in the design as suggested by MPT in his review on the previous merge request, into a phase 3 branch. The only blocker for this branch is the freeze exception.

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

For me code it's fine... In the case you get an exception.

review: Approve

[Bilal Akhtar (bilalakhtar)]

Freeze exception just got the approval, on the linked bug.

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

Tested again, this crashes to me on anther machine.
Here you are the log: http://paste.ubuntu.com/893381/

Please merge with this the branch lp:~3v1n0/unity/favorite-store-crash-fix

review: Needs Fixing

[Bilal Akhtar (bilalakhtar)]

That's a completely unrelated branch, right? That's a problem with trunk, not this branch.

[Tim Penhey (thumper)]

review: Approve

[Unity Merger (unity-merger)]

No commit message specified.

=== modified file 'plugins/unityshell/src/BamfLauncherIcon.h'
--- plugins/unityshell/src/BamfLauncherIcon.h	2012-03-22 15:14:49 +0000
+++ plugins/unityshell/src/BamfLauncherIcon.h	2012-03-28 22:26:18 +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-27 17:27:27 +0000
+++ plugins/unityshell/src/Launcher.cpp	2012-03-28 22:26:18 +0000
@@ -1769,6 +1769,11 @@
   Resize();
 }
 
+int Launcher::GetIconSize() const
+{
+    return _icon_size;
+}
+
 void Launcher::Resize()
 {
   UScreen* uscreen = UScreen::GetDefault();
@@ -2955,7 +2960,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-21 14:44:41 +0000
+++ plugins/unityshell/src/Launcher.h	2012-03-28 22:26:18 +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() const;
 
   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;
@@ -279,8 +285,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-21 19:49:38 +0000
+++ plugins/unityshell/src/LauncherController.cpp	2012-03-28 22:26:18 +0000
@@ -87,9 +87,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);
 
@@ -114,7 +117,7 @@
 
   AbstractLauncherIcon::Ptr CreateFavorite(const char* file_path);
 
-  AbstractLauncherIcon::Ptr CreateSCLauncherIcon(std::string const& file_path, std::string const& aptdaemon_trans_id, std::string const& icon_path);
+  SoftwareCenterLauncherIcon::Ptr CreateSCLauncherIcon(std::string const& file_path, std::string const& aptdaemon_trans_id, std::string const& icon_path);
 
   void SetupBamf();
 
@@ -377,6 +380,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;
@@ -430,24 +435,35 @@
 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)
+  auto bamf_icons = model_->GetSublist<BamfLauncherIcon>();
+  for (auto icon : bamf_icons)
   {
     if (icon->DesktopFile() == path)
       return;
   }
 
-  AbstractLauncherIcon::Ptr result = CreateSCLauncherIcon(path, aptdaemon_trans_id, icon_path);
+  SoftwareCenterLauncherIcon::Ptr result = CreateSCLauncherIcon(path, aptdaemon_trans_id, icon_path);
+
+  launcher_->ForceReveal(true);
+
   if (result)
   {
+    result->SetQuirk(AbstractLauncherIcon::QUIRK_VISIBLE, false);
+    result->Animate(launcher_, icon_x, icon_y, icon_size);
     RegisterIcon(result);
-
-    if (before)
-      model_->ReorderBefore(result, before, false);
+    Save();
   }
-  Save();
+}
+
+void Controller::Impl::OnSCIconAnimationComplete(AbstractLauncherIcon::Ptr icon)
+{
+  icon->SetQuirk(AbstractLauncherIcon::QUIRK_VISIBLE, true);
+  launcher_->ForceReveal(false);
 }
 
 void Controller::Impl::SortAndUpdate()
@@ -733,13 +749,12 @@
   return result;
 }
 
-AbstractLauncherIcon::Ptr
-Controller::Impl::CreateSCLauncherIcon(std::string const& file_path,
-                                       std::string const& aptdaemon_trans_id,
-                                       std::string const& icon_path)
+SoftwareCenterLauncherIcon::Ptr Controller::Impl::CreateSCLauncherIcon(std::string const& file_path,
+                                                                       std::string const& aptdaemon_trans_id,
+                                                                       std::string const& icon_path)
 {
   BamfApplication* app;
-  AbstractLauncherIcon::Ptr result;
+  SoftwareCenterLauncherIcon::Ptr result;
 
   app = bamf_matcher_get_application_for_desktop_file(matcher_, file_path.c_str(), true);
   if (!BAMF_IS_APPLICATION(app))
@@ -751,13 +766,10 @@
     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_++);
+  result = new SoftwareCenterLauncherIcon(app, aptdaemon_trans_id, icon_path);
+  result->SetSortPriority(sort_priority_++);
 
-  result = icon;
   return result;
 }
 

=== modified file 'plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp'
--- plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp	2012-03-28 20:28:50 +0000
+++ plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp	2012-03-28 22:26:18 +0000
@@ -18,29 +18,42 @@
  *              Marco Trevisan (Treviño) <[e-mail]>
  */
 
+#include <NuxCore/Logger.h>
 #include <glib/gi18n-lib.h>
 #include "SoftwareCenterLauncherIcon.h"
+#include "Launcher.h"
+#include "LauncherDragWindow.h"
+#include "LauncherModel.h"
 
 namespace unity
 {
 namespace launcher
 {
 
+NUX_IMPLEMENT_OBJECT_TYPE(SoftwareCenterLauncherIcon);
+
 SoftwareCenterLauncherIcon::SoftwareCenterLauncherIcon(BamfApplication* app,
                                                        std::string const& aptdaemon_trans_id,
                                                        std::string const& icon_path)
 : BamfLauncherIcon(app),
-  _aptdaemon_trans("org.debian.apt",
+  aptdaemon_trans_("org.debian.apt",
                    aptdaemon_trans_id,
                    "org.debian.apt.transaction",
                    G_BUS_TYPE_SYSTEM,
                    G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START)
+, finished_(true)
+, needs_urgent_(false)
 {
-  _aptdaemon_trans.Connect("PropertyChanged", sigc::mem_fun(this, &SoftwareCenterLauncherIcon::OnPropertyChanged));
-  _aptdaemon_trans.Connect("Finished", [&] (GVariant *) {
+
+  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;
+    needs_urgent_ = true;
   });
 
   SetIconType(TYPE_APPLICATION);
@@ -48,8 +61,76 @@
   tooltip_text = _("Waiting to install");
 }
 
-void
-SoftwareCenterLauncherIcon::OnPropertyChanged(GVariant* params)
+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;
+
+  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(),
+                                AbstractLauncherIcon::Ptr(this),
+                                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 bamf_icons = launcher->GetModel()->GetSublist<BamfLauncherIcon>();
+  //TODO: don't iterate through them and pick the last one, just use back() to get the last one.
+  for (auto current_bamf_icon : bamf_icons)
+  {
+    int x = (int) current_bamf_icon->GetCenter(launcher->monitor).x;
+    int y = (int) current_bamf_icon->GetCenter(launcher->monitor).y;
+    if (x != 0 && y != 0)
+    {
+       target_x = x;
+       target_y = y;
+    }
+  }
+
+  target_y = target_y + (launcher->GetIconSize() / 2);
+  drag_window_->SetAnimationTarget(target_x, target_y);
+
+  drag_window_->on_anim_completed = drag_window_->anim_completed.connect(sigc::mem_fun(this, &SoftwareCenterLauncherIcon::OnDragAnimationFinished));
+  drag_window_->StartAnimation();
+}
+
+void SoftwareCenterLauncherIcon::OnDragAnimationFinished()
+{
+  drag_window_->ShowWindow(false);
+  launcher_->icon_animation_complete.emit(AbstractLauncherIcon::Ptr(this));
+  drag_window_ = nullptr;
+}
+
+void SoftwareCenterLauncherIcon::ActivateLauncherIcon(ActionArg arg)
+{
+  if (finished_)
+  {
+      if (needs_urgent_)
+      {
+          SetQuirk(QUIRK_URGENT, false);
+          needs_urgent_ = false;
+      }
+      BamfLauncherIcon::ActivateLauncherIcon(arg);
+  }
+  else
+      SetQuirk(QUIRK_STARTING, false);
+}
+
+void SoftwareCenterLauncherIcon::OnPropertyChanged(GVariant* params)
 {
   gint32 progress;
   glib::String property_name;
@@ -63,12 +144,19 @@
     g_variant_get(property_value, "i", &progress);
 
     if (progress < 100)
+    {
       SetQuirk(QUIRK_PROGRESS, true);
+      finished_ = false;
+    }
 
     SetProgress(progress/100.0f);
     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-03-14 06:24:18 +0000
+++ plugins/unityshell/src/SoftwareCenterLauncherIcon.h	2012-03-28 22:26:18 +0000
@@ -21,25 +21,45 @@
 #ifndef SOFTWARE_CENTER_LAUNCHERICON_H
 #define SOFTWARE_CENTER_LAUNCHERICON_H
 
+#include <UnityCore/GLibDBusProxy.h>
 #include "BamfLauncherIcon.h"
-#include <UnityCore/GLibDBusProxy.h>
+
+class LauncherDragWindow;
 
 namespace unity
 {
 namespace launcher
 {
+class Launcher;
 
 class SoftwareCenterLauncherIcon : public BamfLauncherIcon
 {
+  NUX_DECLARE_OBJECT_TYPE(SoftwareCenterLauncherIcon, BamfLauncherIcon);
 public:
+  typedef nux::ObjectPtr<SoftwareCenterLauncherIcon> Ptr;
+
   SoftwareCenterLauncherIcon(BamfApplication* app,
                              std::string const& aptdaemon_trans_id,
                              std::string const& icon_path);
 
+  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);
-
-  glib::DBusProxy _aptdaemon_trans;
+  void OnDragAnimationFinished();
+
+  glib::DBusProxy aptdaemon_trans_;
+
+  nux::ObjectPtr<nux::IOpenGLBaseTexture> icon_texture_;
+  nux::ObjectPtr<LauncherDragWindow> drag_window_;
+  nux::ObjectPtr<Launcher> launcher_;
+  bool finished_;
+  bool needs_urgent_;
 };
 
 }

=== modified file 'tests/autopilot/autopilot/emulators/X11.py'
--- tests/autopilot/autopilot/emulators/X11.py	2012-03-28 21:22:42 +0000
+++ tests/autopilot/autopilot/emulators/X11.py	2012-03-28 22:26:18 +0000
@@ -338,7 +338,14 @@
         `monitor` must be an integer between 0 and the number of configured monitors.
         ValueError is raised if an invalid monitor is specified.
 
+        BlacklistedDriverError is raised if your video driver does not support this.
+
         """
+        glxinfo_out = subprocess.check_output("glxinfo")
+        for dri in self._blacklisted_drivers:
+            if dri in glxinfo_out:
+                raise ScreenGeometry.BlacklistedDriverError('Impossible change the primary monitor for the given driver')
+
         if monitor < 0 or monitor >= self.get_num_monitors():
             raise ValueError('Monitor %d is not in valid range of 0 <= monitor < %d.' % (self.get_num_monitors()))
 
@@ -347,15 +354,10 @@
         if not monitor_name:
             raise ValueError('Could not get monitor name from monitor number %d.' % (monitor))
 
-        glxinfo_out = subprocess.check_output("glxinfo")
-        for dri in self._blacklisted_drivers:
-            if dri in glxinfo_out:
-                raise ScreenGeometry.BlacklistedDriverError('Impossible change the primary monitor for the given driver')
-
         ret = os.spawnlp(os.P_WAIT, "xrandr", "xrandr", "--output", monitor_name, "--primary")
 
         if ret != 0:
-            raise RuntimeError('Xrandr can\'t set the primary monitor')
+            raise RuntimeError('Xrandr can\'t set the primary monitor. error code: %d' % (ret))
 
     def get_screen_width(self):
         return self._default_screen.get_width()

=== 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-28 22:26:18 +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-25 21:25:54 +0000
+++ tests/autopilot/autopilot/emulators/unity/launcher.py	2012-03-28 22:26:18 +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__)
 
@@ -41,6 +42,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-27 20:48:14 +0000
+++ tests/autopilot/autopilot/tests/test_launcher.py	2012-03-28 22:26:18 +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, multiply_scenarios
@@ -24,7 +24,7 @@
     num_monitors = screen_geometry.get_num_monitors()
 
     if num_monitors == 1:
-        monitor_scenarios =  [('Single Monitor', {'launcher_monitor': 0, 'only_primary': True})]
+        monitor_scenarios =  [('Single Monitor', {'launcher_monitor': 0})]
     else:
         monitor_scenarios = [('Monitor %d' % (i), {'launcher_monitor': i}) for i in range(num_monitors)]
 
@@ -481,6 +481,41 @@
         self.assertThat(self.launcher.key_nav_is_active, Equals(False))
 
 
+    def test_software_center_add_icon(self):
+        """ Test the ability to add a SoftwareCenterLauncherIcon """
+        
+        launcher_instance = self.get_launcher()
+        sc_desktop_file = "/usr/share/applications/ubuntu-software-center.desktop"
+                
+        def cleanup():
+            if icon is not None:
+                launcher_instance.unlock_from_launcher(icon[0])
+
+        # 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."""